-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
nbconvert: Make sure node is atleast version 0.9.12 #5279
Conversation
else: | ||
# Remove the version 'v' prefix from the output and strip whitespace. | ||
version_str = getoutput(cmd + ' --version').replace('v', '').strip() | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See IPython.utils.version.check_version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole block can just be:
try:
out, err, rc = get_output_error_code([cmd, '--version'])
except OSError:
# command not found
return False
if rc:
# command failed
return False
return check_version(out.lstrip('v'), '0.9.12')
@minrk I love the check_version function. I made the changes and pushed, what do you think of having the return_code on the same line as the check_version call (see checked in code). |
Not what I would have done, but it's fine. |
I realize there is one more thing missing in this fallback. In the case of neither node nor pandoc being present, the user will be presented with a 'pandoc missing' error, rather than recommending the preferred installation of 'node' for HTML conversion. I'm not certain we should address that in this PR, but it is suboptimal. |
It's not ideal that this is running subprocesses on import, either. But it was doing that before this change, so it's not critical to fix now. |
I think we can merge this one. Maybe I'll take a pass at cleaning up both node and pandoc checking later if it keeps bothering me. |
It was not launching node before, just locating it (which may have involved calling |
I can add a warning to the else where node isn't found... something like
Also I can move the logic into a method that runs once and caches the results- so subprocess isn't called on import. |
No, what we absolutely must not do is add a warning on import. |
Is that a no to moving it into a method that runs once when the code is called too? I was thinking def markdown2html(source):
"""Convert a markdown string to HTML"""
if _node[0] is None:
# prefer md2html via marked if node.js >= 0.9.12 is available
# node is called nodejs on debian, so try that first
node_cmd = 'nodejs'
if _verify_node(node_cmd):
_node[0] = True
else:
node_cmd = 'node'
if _verify_node(node_cmd):
_node[0] = True
else:
warnings.warn( "Node.js 0.9.12 or later wasn't found.\n" +
"Nbconvert will try to use Pandoc instead.")
_node[0] = False
if not _node[0]:
return markdown2html_pandoc(source)
else:
return markdown2html_marked(source) Pardon the |
Actually like this: def markdown2html(source):
"""Convert a markdown string to HTML"""
if _node[0] is None:
# prefer md2html via marked if node.js >= 0.9.12 is available
# node is called nodejs on debian, so try that first
_node[0] = 'nodejs'
if not _verify_node(_node[0] ):
_node[0] = 'node'
if not _verify_node(_node[0] ):
warnings.warn( "Node.js 0.9.12 or later wasn't found.\n" +
"Nbconvert will try to use Pandoc instead.")
_node[0] = False
if not _node[0]:
return markdown2html_pandoc(source)
else:
return markdown2html_marked(source) |
Something like that should be fine. Except for the |
Because the method isn't child to a class, so I don't have a |
... or we could just not cache the results, and run the check each time |
|
Ahh okay, I was totally confused about the scope of |
global just means module-level |
@takluyver and @minrk want to take a look again? I moved the code into a method and added a warning for the no-node case. |
warnings.warn( "Node.js 0.9.12 or later wasn't found.\n" + | ||
"Nbconvert will try to use Pandoc instead.") | ||
_node = False | ||
if not _node: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not switch the if/else round and do if _node
instead of if not _node
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to switching this one around, otherwise ready to merge, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't sure if there was ever a case where CBool(string) could == False... I guess I was just being overly cautious. Does that bother you? I can rotate them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An empty string booleates to False, but it would work the same whether you test s
or not s
.
Yes, this no longer executes a subcommand on import. 👍 |
Great, this looks good to me now. Thanks, @jdfreder. |
Order switched |
nbconvert: Make sure node is atleast version 0.9.12
nbconvert: Make sure node is atleast version 0.9.12
Trying to export html using nbconvert and an older version of node fails silently. All of the markdown gets stripped from the document. I used
nvm
to install and testnode.js
with ourmarked.js
for node versions 0.6+. I discovered only node versions 0.9.12 and up produce any output. 0.8 and earlier produce no output and 0.9.11 and earlier hang.This PR adds a test that makes sure that node is the right version before deciding to use it, otherwise pandoc is used.
#5263